Remove dependency from $NUPIC environment variable. Fixes numenta/nupic#1815, numenta/nupic#1947 #1970

pehlert · 2015-03-30T02:26:45Z

This is just a suggestion, but maybe it's at least able to point out the places where NuPIC still depends on the env variable

Fixes #1947
~~Blocked by #2006~~

passiweinberger · 2015-03-30T10:08:16Z

Oh oki :) so which approach shall we use now ? the #1968 one or this one ?and then we should close one PR and rather try to see if we can find all occurances and fix them with one approach :)

david-ragazzi · 2015-03-30T11:23:42Z

Nice see new developers contributing.. 😃

cogmission · 2015-03-30T11:54:25Z

👍 ...what @david-ragazzi said!

david-ragazzi · 2015-03-30T13:08:36Z

Hi Pascals,

From the current changes, I see that this PR doesn't remove $NUPIC but attenuate the occurence of os.getenv['NUPIC'] in favor of new NUPIC_ROOT. I like this idea, but in either way, I thought whether would be better get the absolute path from __file__ variable in nupic/init.py and discard $NUPIC once all. @passiweinberger used this in its closed PR. I tried the same thing a time ago but I don't remember well the reasons that I gave up this approach (probably because distutils implementation wasn't done yet). But maybe it's time to try again given the low cost to this change..

david-ragazzi · 2015-03-30T13:17:43Z

nupic/__init__.py

@@ -19,3 +19,7 @@
 # http://numenta.org/licenses/
 # ----------------------------------------------------------------------
 __version__ = "0.3.0.dev0"
+
+import os
+NUPIC_ROOT = os.environ.get('NUPIC', os.path.dirname(os.path.realpath(__file__)))


Have you tried get the absolute path of __file__ and so discard $NUPIC completely?

👍 great idea!

Hi David, my change will consider $NUPIC if it is set or get the path from __file__ as you suggested. I didn't remove $NUPIC altogether because I wanted to avoid side-effects. If you guys are sure nobody relies on it, I'm more than happy to remove it for good. 👍

Let's wait what Matthew thinks ;) keeping it might be better for sidemodules or community projects that may rely on it. There are a lot and Matthew probably has the best overview. How about just a try-except implementation?

-------- Original message --------
From: Pascal Ehlert notifications@github.com
Date:03/30/2015 15:52 (GMT+01:00)
To: numenta/nupic nupic@noreply.github.com
Cc: Pascal Weinberger passiweinberger@gmail.com
Subject: Re: [nupic] Remove dependency from $NUPIC environment variable. Fixes #1815, #1947 (#1970)

In nupic/**init**.py:

@@ -19,3 +19,7 @@

http://numenta.org/licenses/

----------------------------------------------------------------------

version = "0.3.0.dev0"
+
+import os
+NUPIC_ROOT = os.environ.get('NUPIC', os.path.dirname(os.path.realpath(file)))
Hi David, my change will consider $NUPIC if it is set or consider the path from file as you suggested. I didn't remove $NUPIC altogether because I wanted to avoid side-effects. If you guys are sure nobody relies on it, I'm more than happy to remove it for good.

—
Reply to this email directly or view it on GitHub.

You guys are right. We could keep $NUPIC as extra (but not required) variable. So this script should get the value of $NUPIC and in case of it is not found, get the absolute path of this __file__..

I'm not sure whether this is clear, but that's what it does right now. os.environ.get('NUPIC', 'foo') will return either the value of $NUPIC (if it's set) or 'foo'. See https://docs.python.org/2/library/stdtypes.html#dict.get for reference.

So $NUPIC ist completely optional.

Makes sense to me.

So I'll stay with that solution?

passiweinberger · 2015-03-30T17:06:01Z

#1968 now has the NUPIC removed entirely :D so however we decide its there ;)

rhyolight · 2015-03-30T17:45:28Z

I like this. @scottpurdy what do you think?

scottpurdy · 2015-03-30T18:16:47Z

I think this is an improvement. In some cases (all?) we should use pkg_resources rather than paths relative to __file__ in nupic/__init__.py.

One thing that looks off is that the environment variable pointed to the root of the repository while the new logic sets NUPIC_ROOT to the root of the Python package. In some cases this was corrected, like the log path, but in others it isn't. I'll put some comments in for where it looks to me like the paths need to be updated.

Edit: It looks like most places were not updated so I won't keep commenting on all of the instances. One option would be to change NUPIC_ROOT to be the repo root instead of the Python package but I am fine either way.

scottpurdy · 2015-03-30T18:17:41Z

examples/opf/experiments/multistep/base/permutations_simple_3.py

@@ -58,7 +59,7 @@
  'predictedField': 'field2',
  'predictionSteps': [1,3],

-  'dataSource': 'file://%s' % (os.path.join(os.environ['NUPIC'], 'examples', 'opf', 
+  'dataSource': 'file://%s' % (os.path.join(NUPIC_ROOT, 'examples', 'opf', 


examples is in the root of the repo which is a level above NUPIC_ROOT. And pkg_resources would probably be better here but up to you

pehlert · 2015-03-30T18:42:14Z

@scottpurdy I have considered pkg_resources before, but didn't find it appropriate for e.g. setting up the log dir which was the only usage of os.environ['NUPIC'] outside the tests.

As for the log path in general, I agree that it shouldn't default to a directory relative to the package path. Thanks for your feedback, I will give those options a second thought. Do you have any suggestion what we should do about the logs? Maybe os.path.join(os.getcwd(), 'nupic-logs')?

scottpurdy · 2015-03-30T23:01:38Z

If the user doesn't specify a directory for logging then I don't think we should write to a file. I believe the default is to just write to console anyway.

rhyolight · 2015-04-22T12:09:08Z

The major thing we need to decide now that #2006 is merged is: Will NUPIC_ROOT point to the checkout directory or the python package in the nupic directory. To me, NUPIC_ROOT sounds like it would point to the checkout directory, because when I think of the "root" directory in a repository I think of the top-most directory.

The other thing is whether to use pkg_resources or not. I think we should use pkg_resources for all static file references within the codebase. Here is an example of doing this for some config files in setup.py and in the source.

pehlert · 2015-04-22T13:07:07Z

I'm in favor of using pkg_resources whenever possible and only resort to NUPIC_ROOT when there's no other option.

As for the other question, I think this is merely a matter of style. Are there any situations where we need to refer to the "actual root" directory? Otherwise we can safe ourselves some typing and have it point to root/nupic, maybe calling to NUPIC_SRC_ROOT to make this more obvious?

rhyolight · 2015-04-22T13:42:37Z

If there is never a case where we need to refer to the repo root dir, I'm fine leaving it pointing to the nupic module.

rhyolight · 2015-04-28T15:24:00Z

@pehlert ping

pehlert · 2015-05-01T09:10:36Z

@rhyolight Sorry, I've been busy over the last week. I hope to be able to put some effort into this on Sunday.

rhyolight · 2015-05-14T21:32:30Z

@pehlert ping 🍍

david-ragazzi · 2015-06-10T17:15:54Z

@pehlert ping again.. If you still are very busy, maybe @passiweinberger (if he doensn't mind) could incorporate your changes (cherry-picking them) in his PR (#1968) and these $NUPIC variable jobs are done.

passiweinberger · 2015-06-11T09:21:04Z

#2205
Need feedback on @scottpurdy 's comments, I wasn't really sure about the changes here not commented, they seemed to me like they were also referring to the directory one up from NUPIC_ROOT

david-ragazzi · 2015-06-11T11:38:02Z

@pehlert I'm closing this PR because @passiweinberger incorporated your (nice) changes in #2205. Thanks!

david-ragazzi · 2015-06-11T11:40:58Z

#2205
Need feedback on @scottpurdy 's comments, I wasn't really sure about the changes here not commented, they seemed to me like they were also referring to the directory one up from NUPIC_ROOT

Yeah, you are right and see that you addressed well this in #2205. BTW we can close this because @scottpurdy can review the changes there.

Remove dependency from $NUPIC environment variable

636c63a

pehlert changed the title ~~Remove dependency from $NUPIC environment variable~~ Remove dependency from $NUPIC environment variable. Fixes numenta/nupic#1815, numenta/nupic#1947 Mar 30, 2015

pehlert mentioned this pull request Mar 30, 2015

Remove $NUPIC dependency #1968

Closed

passiweinberger mentioned this pull request Mar 30, 2015

Swarming returns JSON error when NUPIC environment variable is unset #1947

Closed

david-ragazzi reviewed Mar 30, 2015
View reviewed changes

Fixed improper reference to NUPIC_ROOT in configuration unit tests

a206231

scottpurdy reviewed Mar 30, 2015
View reviewed changes

rhyolight added the requires author action label Apr 2, 2015

pehlert mentioned this pull request Apr 5, 2015

Make logging to file optional #2005

Merged

rhyolight mentioned this pull request Apr 6, 2015

NuPIC logging to a file should be optional #2006

Closed

rhyolight added status:blocked and removed status:blocked labels Apr 17, 2015

rhyolight added the type:cleanup label Apr 28, 2015

rhyolight assigned pehlert Apr 28, 2015

rhyolight added status:in progress and removed status:in progress requires author action labels May 15, 2015

david-ragazzi mentioned this pull request Jun 11, 2015

Remove $NUPIC 2.0 #2205

Merged

david-ragazzi closed this Jun 11, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dependency from $NUPIC environment variable. Fixes numenta/nupic#1815, numenta/nupic#1947 #1970

Remove dependency from $NUPIC environment variable. Fixes numenta/nupic#1815, numenta/nupic#1947 #1970

pehlert commented Mar 30, 2015

passiweinberger commented Mar 30, 2015

david-ragazzi commented Mar 30, 2015

cogmission commented Mar 30, 2015

david-ragazzi commented Mar 30, 2015

david-ragazzi Mar 30, 2015

breznak Mar 30, 2015 via email

pehlert Mar 30, 2015

passiweinberger Mar 30, 2015

david-ragazzi Mar 30, 2015

pehlert Mar 30, 2015

rhyolight Mar 30, 2015

passiweinberger Jun 11, 2015

passiweinberger commented Mar 30, 2015

rhyolight commented Mar 30, 2015

scottpurdy commented Mar 30, 2015

scottpurdy Mar 30, 2015

pehlert commented Mar 30, 2015

scottpurdy commented Mar 30, 2015

rhyolight commented Apr 22, 2015

pehlert commented Apr 22, 2015

rhyolight commented Apr 22, 2015

rhyolight commented Apr 28, 2015

pehlert commented May 1, 2015

rhyolight commented May 14, 2015

david-ragazzi commented Jun 10, 2015

passiweinberger commented Jun 11, 2015

david-ragazzi commented Jun 11, 2015

david-ragazzi commented Jun 11, 2015

Remove dependency from $NUPIC environment variable. Fixes numenta/nupic#1815, numenta/nupic#1947 #1970

Remove dependency from $NUPIC environment variable. Fixes numenta/nupic#1815, numenta/nupic#1947 #1970

Conversation

pehlert commented Mar 30, 2015

passiweinberger commented Mar 30, 2015

david-ragazzi commented Mar 30, 2015

cogmission commented Mar 30, 2015

david-ragazzi commented Mar 30, 2015

Choose a reason for hiding this comment

breznak Mar 30, 2015 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

----------------------------------------------------------------------

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

passiweinberger commented Mar 30, 2015

rhyolight commented Mar 30, 2015

scottpurdy commented Mar 30, 2015

Choose a reason for hiding this comment

pehlert commented Mar 30, 2015

scottpurdy commented Mar 30, 2015

rhyolight commented Apr 22, 2015

pehlert commented Apr 22, 2015

rhyolight commented Apr 22, 2015

rhyolight commented Apr 28, 2015

pehlert commented May 1, 2015

rhyolight commented May 14, 2015

david-ragazzi commented Jun 10, 2015

passiweinberger commented Jun 11, 2015

david-ragazzi commented Jun 11, 2015

david-ragazzi commented Jun 11, 2015